Codecov Report

Merging #363 (8a2f8ff) into master (549f5cf) will not change coverage.
The diff coverage is n/a.

❗ Current head 8a2f8ff differs from pull request most recent head 597b095. Consider uploading reports for the commit 597b095 to get more accurate results

@@      Coverage Diff      @@
##   master   #363   +/-   ##
=============================
=============================

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 549f5cf...597b095. Read the comment docs.

Merge conflict introduced by recent update to master has been resolved

@myliang and team: ready for your consideration

I see that my PR currently fails the code coverage check. I'm wondering whether this was mostly caused by removing the formula tests (since, in this PR, formula parsing/calculation is now handled by library code rather than x-spreadsheet). I'll see if I can increase the number of tests in general, but I only feel comfortable writing tests for code I touched (and I'm not sure that will bring the test percentage up enough to pass the check).

@myliang : I have improved the test coverage since my last comment. alphabet.js now has all functions covered, and I added tests for locale.js (which previously didn't have them).

Please review at your earliest convenience.

@myliang : is there anything I can do to facilitate a review by you or another project maintainer? I don't want to continue with additional changes until this PR has been reviewed, approved, and merged.

too big changes will not be accepted.

@myliang : I am happy to split this into several smaller PRs:

• chieveit's original formula editing improvements -> Better Formula Editing (by ChieveiT) #382
• npm run test fix -> Make 'npm run test' command cross-platform #380
• locale improvements -> Improve locale flexibility #381
• formula.js conversion
• click and drag cell range selection

Would that be acceptable?

@myliang : I've separated out some of the functionality into smaller PRs:

• npm run test fix -> Make 'npm run test' command cross-platform #380
• locale improvements -> Improve locale flexibility #381

Once those are reviewed, approved, and merged, I'll create small PRs for the other functionality included in this large PR.

@myliang : thanks for the review on 380 and 381. Another smaller PR is now ready for your review:

• chieveit's formula editing improvements -> Better Formula Editing (by ChieveiT) #382

Once that is reviewed, approved, and merged, I'll move on to creating a PR for formula.js integration.

On this branch: https://github.com/jkirschner/x-spreadsheet/tree/formulajs-range-selection

@myliang and @jkirschner this is a great feature update!

This makes it more like how Excel works.
Another good feature is dragging and dropping cells from one place to another. Very common feature.

Thanks.

Thanks @mm738. Trying to work with the maintainers to get this integrated upstream in parts (since this PR was too large). So far we are 2/5 of the way there!

jkirschner this is a great improvement wrt formula calculations within the worksheet. With the old implementation, simple calculations resulted in erroneous values. However, your formulajs branch in its current state breaks document printing. The references to "this" formulaParser within the renderCell function call are invalid in src/component/table.js

I report a possible bug for @jkirschner here (hope this is the place)
On this branch: https://github.com/jkirschner/x-spreadsheet/tree/formulajs-range-selection

If I use the formula selector, any formula will write "=undefined()" in the cell instead of the correct formula.

Note that it's still possible to select formulas from the cell completion suggestions.

This is a very powerful feature and I'm sad to see is not progressing - what can be done to help?

@pbadenski, @harlan3 : I started breaking this large PR up into smaller PRs upon request by the project owner. One of these smaller PRs is still awaiting review/approval/merge: #382. Perhaps the next step is getting that PR through.

Unfortunately, I have a lot less time now than I did when I originally created this PR. I can't push it forward right now (e.g., fix the printing bug), but I do want to come back to it eventually.

@pbadenski, @harlan3, @ThibautSF, @myliang : I have some time over the next week to push this PR forward and hopefully get it through the review process. My understanding from the maintainer is that it needs to be broken up into smaller PRs which can then be individually reviewed and merged. I'm partway through that process: the latest PRs to review are #382, #464, #465, and #466.

@jkirschner Thanks for the effort but I've tested this PR a lot and there fundamental issues with it that cannot be solved easily. Just saying cause don't want you to waste your time.

1. x-datasheet doesn't handle smart formula parsing. I.e it loops through all cells in a linear order, it does not build any graphs. This means when changing cell dependents it wont update cells in the correct order. I'm not sure if hot-formula parser does build the graph correctly but when I tested this PR it was very laggy.
2. The parsing of the formulas in this PR is done on every render of the cell. This isn't efficient. It should be done once and cached and not on every render.

Due to the above issues the spreadsheets lags with any reasonable amount of formulas or circle references.

I have been using the @jkirschner branch for several months now (along with some additional improvements for loading and saving files) and I would say it is vastly superior in comparison to the x-spreadsheet that doesn't have smart formula parsing. I have used it with spreadsheets spanning several hundred cells and it does slow down some but for a "normal" sized spreadsheet it is fine. If you have something better then you should submit it back to git master and the code should fall under the MIT license as the original project uses the MIT. It isn't right that you leverage all the code to x-spreadsheet and then not contribute your work back to the project. That is completely unethical. I am perfectly happy using the @jkirschner branch. It is just unfortunate that it has not been promoted into the git master yet.

And just at a UX level, this PR is way better. Formula edition alongside absolute cell reference gives a better end-user experience, near enough of what you can expect on a desktop sheet and simple enough for a integrated web use (didn't find anything better for now in other open source sheet libraries).

X-spreadsheet is really good, this PR made it just better. It sure has drawbacks in case of big sheets, but those can be solved with code review, suggestions and improvements.

@jkirschner Thanks for the effort but I've tested this PR a lot and there fundamental issues with it that cannot be solved easily. Just saying cause don't want you to waste your time.

1. x-datasheet doesn't handle smart formula parsing. I.e it loops through all cells in a linear order, it does not build any graphs. This means when changing cell dependents it wont update cells in the correct order. I'm not sure if hot-formula parser does build the graph correctly but when I tested this PR it was very laggy.
2. The parsing of the formulas in this PR is done on every render of the cell. This isn't efficient. It should be done once and cached and not on every render.

Due to the above issues the spreadsheets lags with any reasonable amount of formulas or circle references.

@MartinDawson : Can you help me understand how either of those concerns is related to this PR? If I understand correctly, (1) looping through all cells in a linear order and (2) parsing formulas on every render of the cell were both behaviors of x-spreadsheet before this PR. This PR changes the parsing engine for formulas, but not when parsing is performed.

Yes, improvements to efficiency can be made, but those should be done in a separate PR.

@harlan3, @ThibautSF: because of @MartinDawson 's comments, I put together a proof-of-concept that builds on this PR to (1) perform efficient recalculations and (2) detect circular references: https://github.com/jkirschner/x-spreadsheet/tree/efficient-recalculation

Let me know if you try that branch out and see a significant enough performance improvement that it's worth me finishing the branch. I still need to make some small fixes (such as to print mode) and significantly cleanup the commit history via rebasing, so I wouldn't use the efficient-recalculation branch yet... but it's ready to try out!

@harlan3, @ThibautSF: because of @MartinDawson 's comments, I put together a proof-of-concept that builds on this PR to (1) perform efficient recalculations and (2) detect circular references: https://github.com/jkirschner/x-spreadsheet/tree/efficient-recalculation

Let me know if you try that branch out and see a significant enough performance improvement that it's worth me finishing the branch. I still need to make some small fixes (such as to print mode) and significantly cleanup the commit history via rebasing, so I wouldn't use the efficient-recalculation branch yet... but it's ready to try out!

I tried it with some minimal testing, and it seemed to work ok. To really test it I would need to merge in my spreadsheet file loading and saving capability which would allow the loading of large x-spreadsheet files. If you are interested in putting the file loading and saving functionality in your main baseline let me know and I will create a branch for a PR.

I tried it with some minimal testing, and it seemed to work ok. To really test it I would need to merge in my spreadsheet file loading and saving capability which would allow the loading of large x-spreadsheet files. If you are interested in putting the file loading and saving functionality in your main baseline let me know and I will create a branch for a PR.

@harlan3 : I probably won't merge any PRs, including my own, into my fork's main branch because I'm trying to keep that consistent with upstream - which relies on getting things merged upstream. If you submit a PR against the main branch, I'll probably take those commits and place them on top of efficient-recalculation to create a new branch that you can test in. Or, you could create a PR against efficient-recalculation yourself!

It's worth doing that so we can test the performance improvements and see if it's worth pursuing further. Thanks!

I tried it with some minimal testing, and it seemed to work ok. To really test it I would need to merge in my spreadsheet file loading and saving capability which would allow the loading of large x-spreadsheet files. If you are interested in putting the file loading and saving functionality in your main baseline let me know and I will create a branch for a PR.

@harlan3 : I probably won't merge any PRs, including my own, into my fork's main branch because I'm trying to keep that consistent with upstream - which relies on getting things merged upstream. If you submit a PR against the main branch, I'll probably take those commits and place them on top of efficient-recalculation to create a new branch that you can test in. Or, you could create a PR against efficient-recalculation yourself!

It's worth doing that so we can test the performance improvements and see if it's worth pursuing further. Thanks!

I created a fork of your x-spreadsheet repo and merged in the changes for file loading and saving in my efficient-recalculation branch. You can clone it from here: https://github.com/harlan3/x-spreadsheet/tree/efficient-recalculation
To use the new feature, click on the "pen" icon which is on the far right of the toolbar. This toggles between the spreadsheet view and the JSON view of the table. To load the spreadsheet, copy and paste the contents of one of the files in the new sample_files directory such as loan_calc.txt. It is a pretty large file and I think your performance changes in this branch have helped out considerably.

@jkirschner @harlan3 : is there any way to get parsed value programmatic ? likecellParsedValue(2,6)

@jkirschner @harlan3 : is there any way to get parsed value programmatic ? likecellParsedValue(2,6)

I made this function (based on x-spreadsheet code and using the xspreadsheet cell(rowIndex, colIndex, sheetIndex) method) in order to get parsed value for any cell.

function getCellValue(xspreadsheet, rowIndex, colIndex, sheetIndex) {
    if (xspreadsheet) {
        if ((cell = xspreadsheet.cell(rowIndex, colIndex, sheetIndex))) {
            var cellText = cell.text;

            if (cellText && cellText[0] == "=") {
                cellText = xspreadsheet.sheet.table.formulaParser.parse(
                    cellText.slice(1)
                );

                cellText = cellText.result;
            }

            return Number(cellText) || cellText;
        }
    }
}

Note: xspreadsheet is the xspreadsheet instance object.

This should've been merged a long time ago, such splendid work!

This works fine... can you rebase and resolve the conflicts? We can supersed if not :)

Hi @Tardo,

The maintainer previously said they would not accept this PR as is because it was too large. That's very understandable, so I started the process of breaking it into many smaller PRs. The first few sub-PRs were merged. But the next few sub-PRs have not been reviewed. And there's a lot of work in this branch that hasn't made it into a sub-PR yet, because I was waiting for the existing sub-PRs to be merged.

A lot of time has since passed.

I cannot justify spending more time on this unless the maintainer expresses interest in this PR. And if that happens, I'm not sure I even have the bandwidth to pick this back up right now.

I really appreciate the feedback and collaboration I've had with the community members in this thread. I wish you the best of luck with your spreadsheets :)

@jkirschner Thank you for your response and the work made here. Too bad the creator doesn't want to review/merge the PR... what matters are the commits :/ It's good to know how eager the maintainer is to include improvements in his repo...

Guys, just use this canvas spreadsheet: https://github.com/glideapps/glide-data-grid

It's far better and not got 10,000 bugs in it.

And just use hyperformula as your cell engine parser

@smazydev how can I use xspreadsheet.js file from this pull request? Please help.